Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

32669 sumconnector config #33994

Merged
merged 22 commits into from
Jul 25, 2024
Merged

32669 sumconnector config #33994

merged 22 commits into from
Jul 25, 2024

Conversation

greatestusername
Copy link
Contributor

@greatestusername greatestusername commented Jul 9, 2024 •
edited by crobert-1
Loading

Description: Fleshes out the config more fully and provides tests for the config and possible combinations of settings

Link to tracking Issue: #32669

Testing: Added test cases and testing for config

Documentation: No new documentation yet.

@greatestusername
Copy link
Contributor Author

greatestusername commented Jul 9, 2024 •
edited
Loading

Regarding tests and the size of PRs:

Is it cool to do tests in a separate PR from functionality?
Realizing that the tests for this small bit were like 500 lines and I'm sure you don't want to look at things much larger than that in an entire PR if possible 😛

Edit to add: Is changelog required here if the component isn't live or wired in yet?

@greatestusername greatestusername marked this pull request as ready for review July 9, 2024 19:02
@greatestusername greatestusername requested a review from a team July 9, 2024 19:02
@crobert-1
Copy link
Member

crobert-1 commented Jul 9, 2024 •
edited
Loading

Regarding tests and the size of PRs:

This is fine for me since it's mostly test code. My preference is to keep functionality changes and the relevant tests in the same PR as much as possible. I'll review.

As far as changelog, it's really just what's relevant to users. Announcing that a new component is being adding makes sense (the last PR), adding config validation not as much. I don't think a changelog is required here, but you're welcome to add one if you'd like.

@crobert-1 crobert-1 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 9, 2024
crobert-1
crobert-1 reviewed Jul 9, 2024
View reviewed changes
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Show resolved Hide resolved
crobert-1
crobert-1 reviewed Jul 10, 2024
View reviewed changes
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Outdated Show resolved Hide resolved
connector/sumconnector/config.go Show resolved Hide resolved
@crobert-1
Copy link
Member

From failing CI actions:

Generated code is out of date, please run "make generate" and commit the changes in this PR.

sumconnector/config_test.go:510: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `spans: metric name missing`+"\n"+`spans: metric source_attribute missing`+"\n"+`spans condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`spans attributes: metric "": attribute key missing`,
sumconnector/config_test.go:525: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `spanevents: metric name missing`+"\n"+`spanevents: metric source_attribute missing`+"\n"+`spanevents condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`spanevents attributes: metric "": attribute key missing`,
sumconnector/config_test.go:540: File is not `gci`-ed with --skip-generated -s standard -s default -s prefix(github.com/open-telemetry/opentelemetry-collector-contrib) (gci)
			expect: `metrics: metric name missing`+"\n"+`metrics: metric source_attribute missing`+"\n"+`metrics condition: metric "": unable to parse OTTL condition "invalid condition": condition has invalid syntax: 1:9: unexpected token "condition" (expected <opcomparison> Value)`+"\n"+`metrics attributes not supported: metric ""`,

@crobert-1 crobert-1 mentioned this pull request Jul 16, 2024
Closed
@crobert-1
Copy link
Member

Test failures are frequencies of #33998, added reference there.

@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jul 16, 2024
@github-actions github-actions bot mentioned this pull request Jul 23, 2024
Closed
@codeboten codeboten merged commit 358ab68 into open-telemetry:main Jul 25, 2024
154 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crobert-1 crobert-1 crobert-1 approved these changes

@atoulme atoulme atoulme approved these changes

@shalper2 shalper2 Awaiting requested review from shalper2

Assignees

@bogdandrutu bogdandrutu

Labels
connector/sum ready to merge Code review completed; ready to merge by maintainers Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Milestone
v0.106.0
Development

Successfully merging this pull request may close these issues.
5 participants
@greatestusername @crobert-1 @atoulme @codeboten @bogdandrutu